repl: handle errors from getters during completion #59044

islandryu · 2025-07-13T07:07:49Z

No description provided.

codecov · 2025-07-13T07:56:24Z

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.07%. Comparing base (b16a0e7) to head (11222b5).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/repl.js	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59044      +/-   ##
==========================================
+ Coverage   90.05%   90.07%   +0.01%     
==========================================
  Files         645      645              
  Lines      189153   189162       +9     
  Branches    37091    37099       +8     
==========================================
+ Hits       170348   170380      +32     
+ Misses      11528    11478      -50     
- Partials     7277     7304      +27

Files with missing lines	Coverage Δ
lib/repl.js	`94.24% <91.66%> (+0.02%)`	⬆️

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

islandryu · 2025-07-13T08:18:49Z

I noticed this while trying to add a test for a getter in the following PR, it seems that if an error is thrown inside a getter, the server itself crashes.
#58943

@dario-piotrowicz
I'd appreciate it if you could take a look.

dario-piotrowicz

@islandryu thanks so much for spotting the issue 🙏

The change looks good to me 😃

But I think the test can be cleaned up a bit (it looks more complex than it needs to me in my opinion), in it I see:

the unnecessary definition of a class
unnecessary multiple evals
missing spacing
duplicated code

I would suggest to change the test to something like the following instead:

'use strict';

const common = require('../common');
const repl = require('repl');
const ArrayStream = require('../common/arraystream');
const assert = require('assert');

(async function() {
 await runTest();
})().then(common.mustCall());

async function runTest() {
 const input = new ArrayStream();
 const output = new ArrayStream();

 const replServer = repl.start({
   prompt: '',
   input,
   output: output,
   allowBlockingCompletions: true,
   terminal: true
 });

 replServer._domain.on('error', (e) => {
   assert.fail(`Error in REPL domain: ${e}`);
 });

 await new Promise((resolve, reject) => {
   replServer.eval(`
     const getNameText = () => "name";
     const foo = { get name() { throw new Error(); } };
     `, replServer.context, '', (err) => {
     if (err) {
       reject(err);
     } else {
       resolve();
     }
   });
 });

 ['foo.name.', 'foo["name"].', 'foo[getNameText()].'].forEach(test => {
   replServer.complete(
     test,
     common.mustCall((error, data) => {
       assert.strictEqual(error, null);
       assert.strictEqual(data.length, 2);
       assert.strictEqual(data[1], test);
     })
   );
 })
}

But besides the test the change looks great to me 😄

islandryu · 2025-07-13T11:48:50Z

Thanks for the feedback!
Indeed, my test was unnecessarily complex.
I've updated it following your suggestion!

dario-piotrowicz

Thanks a lot for updating the test @islandryu 🫶

Looks good to me, again thanks a lot for spotting this! 🚀

nodejs-github-bot · 2025-07-15T23:08:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/67948/

nodejs-github-bot · 2025-07-17T05:25:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/67973/

nodejs-github-bot · 2025-07-17T11:31:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/67978/

nodejs-github-bot · 2025-07-18T06:32:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68000/

nodejs-github-bot · 2025-07-19T01:50:26Z

Commit Queue failed

- Loading data for nodejs/node/pull/59044
✔  Done loading data for nodejs/node/pull/59044
----------------------------------- PR info ------------------------------------
Title      repl: handle errors from getters during completion (#59044)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     islandryu:fix/repl-complete-getter-error -> nodejs:main
Labels     repl, needs-ci, commit-queue-squash
Commits    2
 - repl: handle errors from getters during completion
 - fix test
Committers 1
 - islandryu <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/59044
Reviewed-By: Dario Piotrowicz <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/59044
Reviewed-By: Dario Piotrowicz <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 13 Jul 2025 07:07:49 GMT
   ✔  Approvals: 1
   ✔  - Dario Piotrowicz (@dario-piotrowicz): https://github.com/nodejs/node/pull/59044#pullrequestreview-3014134305
   ✘  This PR needs to wait 29 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-07-18T06:32:35Z: https://ci.nodejs.org/job/node-test-pull-request/68000/
- Querying data for job/node-test-pull-request/68000/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/16383640289

dario-piotrowicz · 2025-07-19T11:14:27Z

oh.... this needs one more day before it can be merged 😓

dario-piotrowicz · 2025-07-19T12:21:03Z

ah alright, thanks James 🙂

nodejs-github-bot · 2025-07-19T12:32:21Z

Landed in c8d5b39

PR-URL: #59044 Reviewed-By: Dario Piotrowicz <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Jul 13, 2025

islandryu marked this pull request as draft July 13, 2025 07:13

islandryu force-pushed the fix/repl-complete-getter-error branch 2 times, most recently from 7852293 to eacb7f6 Compare July 13, 2025 08:12

islandryu marked this pull request as ready for review July 13, 2025 08:13

dario-piotrowicz reviewed Jul 13, 2025

View reviewed changes

dario-piotrowicz approved these changes Jul 13, 2025

View reviewed changes

islandryu added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 13, 2025

islandryu mentioned this pull request Jul 13, 2025

repl: catch promise errors during eval in completion #58943

Open

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 15, 2025

islandryu added 2 commits July 18, 2025 15:30

repl: handle errors from getters during completion

9fbd211

fix test

11222b5

islandryu force-pushed the fix/repl-complete-getter-error branch from a12247b to 11222b5 Compare July 18, 2025 06:31

islandryu added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 19, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 19, 2025

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 19, 2025

dario-piotrowicz added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 19, 2025

jasnell approved these changes Jul 19, 2025

View reviewed changes

jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 19, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 19, 2025

nodejs-github-bot merged commit c8d5b39 into nodejs:main Jul 19, 2025
67 checks passed

aduh95 pushed a commit that referenced this pull request Jul 21, 2025

repl: handle errors from getters during completion

1a44265

PR-URL: #59044 Reviewed-By: Dario Piotrowicz <[email protected]> Reviewed-By: James M Snell <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

repl: handle errors from getters during completion #59044

repl: handle errors from getters during completion #59044

Uh oh!

islandryu commented Jul 13, 2025

Uh oh!

codecov bot commented Jul 13, 2025 •

edited

Loading

Uh oh!

islandryu commented Jul 13, 2025

Uh oh!

dario-piotrowicz left a comment •

edited

Loading

Uh oh!

islandryu commented Jul 13, 2025

Uh oh!

dario-piotrowicz left a comment

Uh oh!

nodejs-github-bot commented Jul 15, 2025

Uh oh!

nodejs-github-bot commented Jul 17, 2025

Uh oh!

nodejs-github-bot commented Jul 17, 2025

Uh oh!

nodejs-github-bot commented Jul 18, 2025

Uh oh!

nodejs-github-bot commented Jul 19, 2025

Uh oh!

dario-piotrowicz commented Jul 19, 2025

Uh oh!

dario-piotrowicz commented Jul 19, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 19, 2025

Uh oh!

Uh oh!

Uh oh!

repl: handle errors from getters during completion #59044

repl: handle errors from getters during completion #59044

Uh oh!

Conversation

islandryu commented Jul 13, 2025

Uh oh!

codecov bot commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

islandryu commented Jul 13, 2025

Uh oh!

dario-piotrowicz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

islandryu commented Jul 13, 2025

Uh oh!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 15, 2025

Uh oh!

nodejs-github-bot commented Jul 17, 2025

Uh oh!

nodejs-github-bot commented Jul 17, 2025

Uh oh!

nodejs-github-bot commented Jul 18, 2025

Uh oh!

nodejs-github-bot commented Jul 19, 2025

Uh oh!

dario-piotrowicz commented Jul 19, 2025

Uh oh!

dario-piotrowicz commented Jul 19, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 19, 2025

Uh oh!

Uh oh!

codecov bot commented Jul 13, 2025 •

edited

Loading

dario-piotrowicz left a comment •

edited

Loading